Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to add iomap to TSS (take 2) #194

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Restioson
Copy link
Member

@Restioson Restioson commented Oct 17, 2020

I've refactored #68 to add a new method which allows you to specify IO permissions bitmap size, rather than incorporating it into the existing method. The existing method is now implemented by calling the new, unsafe method with an io permissions bitmap size of 0.

I've tested this by allowing usermode to access the first serial point and then trying to print, which does work. I'm not sure what kind of other unit tests I should write for this.

I've left out any kind of IoPermissionsBitmap structure for now, as this can be implemented on top of this change, and I'm not quite sure about the specifics (for instance, what if you don't want the full 8192 bytes?).

src/structures/gdt.rs Outdated Show resolved Hide resolved

/// Creates a TSS system descriptor for the given TSS, setting up the IO permissions bitmap.
///
/// # Safety
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could put enough checks here to make this method safe. We could take the IO map as a [u8] slice and then do all of the necessary checks (it's in range, byte past the end is all 1s, iomap_base is <= 0xDFFFH, etc..). We either need to do these checks or document all the requirements from the SDM in the # Safety section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a result if it fails the checks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that or panic. We would need a custom error type for that (it could live in the tss module).

Copy link
Member Author

@Restioson Restioson Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I've expanded on the safety requirements. I couldn't find any others than terminating 0xff byte, iomap_base <= 0xdfff, and it points to the IO map slice in the SDM, although maybe I missed some.

Copy link
Member Author

@Restioson Restioson Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be possible to have a TssWithIoMap struct, as discussed here, but I'm unsure how non-8192-length io permissions bitmaps should be handled. Otherwise, that could also be safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later on, we could add a structure (the TssWithIoMap) to this library. That structure would make sure things stay valid (iomap_addr is correctly initialized, there's always a trailing 0xff byte, etc...) And we'd have a descriptor method on that structure that calls this method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should just check that the iomap_base is correct, and return an error if it's wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I've pushed a prototype of this now.

Copy link
Member Author

@Restioson Restioson Oct 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking over this change again, and I think it has pretty good potential to make mutation of the IO map impossible. As I understand it, it should be possible to change the IO permissions bitmap at runtime, or am I mistaken? If so, giving &'static [u8] would make any modifications instant UB. Perhaps it could take &'static [UnsafeCell<u8>] to get around this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first part of this conversation seems to be resolved.

I'm thinking over this change again, and I think it has pretty good potential to make mutation of the IO map impossible. As I understand it, it should be possible to change the IO permissions bitmap at runtime, or am I mistaken? If so, giving &'static [u8] would make any modifications instant UB. Perhaps it could take &'static [UnsafeCell<u8>] to get around this?

It's probably best to merge it as is and look into this in a separate PR. I'm not sure whether UnsafeCell would be enough for this or whether we need to e.g. use AtomicU8. We should ask that someone who has more knowledge of the Rust compiler.

src/structures/gdt.rs Outdated Show resolved Hide resolved
src/structures/gdt.rs Outdated Show resolved Hide resolved
@Restioson
Copy link
Member Author

Maybe it would be possible to write an integration test for this wherein the kernel is dropped to userspace and tests out a few ports? I can't get the integration tests to compile though so I'm unsure how to proceed.

@Restioson
Copy link
Member Author

Argh, I should really set up cargo fmt as a pre-commit hook or something.

return Err(InvalidIoMap::TooLong { len: iomap.len() });
}

let base = iomap.as_ptr() as usize - tss as *const _ as usize;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to use wrapping_offset_from here and get an isize. We need to deal with isizes because the iomap could come before the tss, and that would also be wrong.

Copy link
Member Author

@Restioson Restioson Oct 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrapping_offset_from doesn't exist. I think that offset_from would also be incorrect, as according to offset_from's safety contract:

The distance between the pointers, in bytes, cannot overflow an isize.

This is overflowed if TSS is higher half (say, 0xffffffff80000000) and IO permissions bitmap is lower half (say, 0x1000), I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just handled this with a manual if check.

@phil-opp
Copy link
Member

@josephlr Do you have time to continue your review or should I try to take over?

@phil-opp phil-opp added the waiting-for-review Waiting for a review from the maintainers. label Dec 28, 2020
@phil-opp
Copy link
Member

phil-opp commented Apr 1, 2021

@Restioson Sorry for the long silence! It seems like @josephlr does no longer have time to review this, so I'll take a look.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks a lot! This only needs a rebase, then it should be ready for merging. We can look into the modification problem in a follow-up PR.


/// The given IO permissions bitmap is invalid.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum InvalidIoMap {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to have a Display implementation for this struct as a quick way to print an error.

src/structures/tss.rs Show resolved Hide resolved
src/structures/gdt.rs Show resolved Hide resolved

/// Creates a TSS system descriptor for the given TSS, setting up the IO permissions bitmap.
///
/// # Safety
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first part of this conversation seems to be resolved.

I'm thinking over this change again, and I think it has pretty good potential to make mutation of the IO map impossible. As I understand it, it should be possible to change the IO permissions bitmap at runtime, or am I mistaken? If so, giving &'static [u8] would make any modifications instant UB. Perhaps it could take &'static [UnsafeCell<u8>] to get around this?

It's probably best to merge it as is and look into this in a separate PR. I'm not sure whether UnsafeCell would be enough for this or whether we need to e.g. use AtomicU8. We should ask that someone who has more knowledge of the Rust compiler.

@Restioson
Copy link
Member Author

Thanks for the review, I will try add the display impl and rebase it shortly! Right now I have a bit of university work to finish but hopefully soon after I can get to this.

@phil-opp phil-opp added waiting-on-author Waiting for the author to act on review feedback. and removed waiting-for-review Waiting for a review from the maintainers. labels Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-on-author Waiting for the author to act on review feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants